RHINENG-25944: add search to the tags list endpoint#2204
Conversation
Allow `/tags` to filter returned tags by matching the search term against tag namespace, key, and value. With this the `/tags` endpoint could be used in the frontend for tag filtering.
Reviewer's GuideAdds search support to the /tags listing endpoint by wiring search fields into the generic list options, documenting the new query parameter, and covering it with tests for matching and non-matching queries. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="manager/controllers/systemtags.go" line_range="52-55" />
<code_context>
},
StableSort: "tag",
DefaultSort: "tag",
+ SearchFields: []string{
+ "sq.tag->>'namespace'",
+ "sq.tag->>'key'",
+ "sq.tag->>'value'",
+ },
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid hard-coding the SQL alias in `SearchFields` to reduce coupling and future breakage risk
These `SearchFields` are tightly coupled to the `sq` alias (for example, `"sq.tag->>'namespace'"`). If the table/CTE alias changes, search will silently break. Either make these expressions independent of the outer alias (if supported), or centralize them in a shared constant/helper used by both the query builder and `SearchFields` so alias updates are done in one place.
Suggested implementation:
```golang
SearchFields: []string{
"tag->>'namespace'",
"tag->>'key'",
"tag->>'value'",
},
```
This change assumes that the query builder / underlying SQL does not require an explicit table/CTE alias prefix for these expressions. If the current implementation *does* require the alias (e.g. because multiple tables expose a `tag` column), you should instead:
1. Introduce a shared constant or helper (e.g. `const systemTagAlias = "sq"` or a function that formats search fields for a given alias) in this file or a shared package.
2. Use that constant/helper both where the alias is defined in the SQL/CTE and here in `SearchFields`, so alias changes are centralized.
</issue_to_address>
### Comment 2
<location path="manager/controllers/systemtags_test.go" line_range="81-90" />
<code_context>
assert.Equal(t, 400, w.Code)
}
+
+func TestSystemTagsListSearch(t *testing.T) {
+ core.SetupTest(t)
+
+ w := CreateRequestRouterWithAccount("GET", "/", "", "?search=k3", nil, "", SystemTagListHandler, 1)
+
+ var output SystemTagsResponse
+ CheckResponse(t, w, http.StatusOK, &output)
+
+ assert.Equal(t, 2, len(output.Data))
+ assert.Equal(t, 2, output.Meta.TotalItems)
+ assert.Equal(t, "k3", output.Meta.Search)
+
+ assert.Equal(t, 1, output.Data[0].Count)
+ assert.Equal(t, "k3", output.Data[0].Tag.Key)
+ assert.Equal(t, "ns1", output.Data[0].Tag.Namespace)
+ assert.Equal(t, "val3", output.Data[0].Tag.Value)
+
+ assert.Equal(t, 3, output.Data[1].Count)
+ assert.Equal(t, "k3", output.Data[1].Tag.Key)
+ assert.Equal(t, "ns1", output.Data[1].Tag.Namespace)
+ assert.Equal(t, "val4", output.Data[1].Tag.Value)
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid relying on result ordering and tighten assertions to better express expectations
This test is tightly coupled to the current result order and exact per-index counts/values, so it may become brittle if query or default sorting changes while behavior remains correct. Either (a) assert that both expected tag combinations are present regardless of order (e.g., by sorting or mapping the response), or (b) treat sort order as part of the contract and assert on the sort field explicitly. Also consider asserting `output.Meta.TotalItems` relative to `len(output.Data)` to ensure pagination metadata matches the returned items.
Suggested implementation:
```golang
import (
"fmt"
"net/http"
"testing"
"app/base/core"
"github.com/stretchr/testify/assert"
)
```
```golang
func TestSystemTagsListSearch(t *testing.T) {
core.SetupTest(t)
w := CreateRequestRouterWithAccount("GET", "/", "", "?search=k3", nil, "", SystemTagListHandler, 1)
var output SystemTagsResponse
CheckResponse(t, w, http.StatusOK, &output)
// Expected tag combinations and counts, independent of result order.
expected := map[string]int{
"ns1:k3:val3": 1,
"ns1:k3:val4": 3,
}
assert.Equal(t, len(expected), len(output.Data))
assert.Equal(t, len(expected), output.Meta.TotalItems)
assert.Equal(t, "k3", output.Meta.Search)
seen := make(map[string]bool)
for _, item := range output.Data {
key := fmt.Sprintf("%s:%s:%s", item.Tag.Namespace, item.Tag.Key, item.Tag.Value)
expectedCount, ok := expected[key]
assert.True(t, ok, "unexpected tag combination in response: %s", key)
assert.Equal(t, expectedCount, item.Count, "unexpected count for tag %s", key)
seen[key] = true
}
// Ensure all expected tag combinations were returned.
for key := range expected {
assert.True(t, seen[key], "expected tag combination not found in response: %s", key)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| SearchFields: []string{ | ||
| "sq.tag->>'namespace'", | ||
| "sq.tag->>'key'", | ||
| "sq.tag->>'value'", |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid hard-coding the SQL alias in SearchFields to reduce coupling and future breakage risk
These SearchFields are tightly coupled to the sq alias (for example, "sq.tag->>'namespace'"). If the table/CTE alias changes, search will silently break. Either make these expressions independent of the outer alias (if supported), or centralize them in a shared constant/helper used by both the query builder and SearchFields so alias updates are done in one place.
Suggested implementation:
SearchFields: []string{
"tag->>'namespace'",
"tag->>'key'",
"tag->>'value'",
},This change assumes that the query builder / underlying SQL does not require an explicit table/CTE alias prefix for these expressions. If the current implementation does require the alias (e.g. because multiple tables expose a tag column), you should instead:
- Introduce a shared constant or helper (e.g.
const systemTagAlias = "sq"or a function that formats search fields for a given alias) in this file or a shared package. - Use that constant/helper both where the alias is defined in the SQL/CTE and here in
SearchFields, so alias changes are centralized.
| func TestSystemTagsListSearch(t *testing.T) { | ||
| core.SetupTest(t) | ||
|
|
||
| w := CreateRequestRouterWithAccount("GET", "/", "", "?search=k3", nil, "", SystemTagListHandler, 1) | ||
|
|
||
| var output SystemTagsResponse | ||
| CheckResponse(t, w, http.StatusOK, &output) | ||
|
|
||
| assert.Equal(t, 2, len(output.Data)) | ||
| assert.Equal(t, 2, output.Meta.TotalItems) |
There was a problem hiding this comment.
suggestion (testing): Avoid relying on result ordering and tighten assertions to better express expectations
This test is tightly coupled to the current result order and exact per-index counts/values, so it may become brittle if query or default sorting changes while behavior remains correct. Either (a) assert that both expected tag combinations are present regardless of order (e.g., by sorting or mapping the response), or (b) treat sort order as part of the contract and assert on the sort field explicitly. Also consider asserting output.Meta.TotalItems relative to len(output.Data) to ensure pagination metadata matches the returned items.
Suggested implementation:
import (
"fmt"
"net/http"
"testing"
"app/base/core"
"github.com/stretchr/testify/assert"
)func TestSystemTagsListSearch(t *testing.T) {
core.SetupTest(t)
w := CreateRequestRouterWithAccount("GET", "/", "", "?search=k3", nil, "", SystemTagListHandler, 1)
var output SystemTagsResponse
CheckResponse(t, w, http.StatusOK, &output)
// Expected tag combinations and counts, independent of result order.
expected := map[string]int{
"ns1:k3:val3": 1,
"ns1:k3:val4": 3,
}
assert.Equal(t, len(expected), len(output.Data))
assert.Equal(t, len(expected), output.Meta.TotalItems)
assert.Equal(t, "k3", output.Meta.Search)
seen := make(map[string]bool)
for _, item := range output.Data {
key := fmt.Sprintf("%s:%s:%s", item.Tag.Namespace, item.Tag.Key, item.Tag.Value)
expectedCount, ok := expected[key]
assert.True(t, ok, "unexpected tag combination in response: %s", key)
assert.Equal(t, expectedCount, item.Count, "unexpected count for tag %s", key)
seen[key] = true
}
// Ensure all expected tag combinations were returned.
for key := range expected {
assert.True(t, seen[key], "expected tag combination not found in response: %s", key)
}
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2204 +/- ##
=======================================
Coverage 59.23% 59.23%
=======================================
Files 137 137
Lines 8795 8795
=======================================
Hits 5210 5210
Misses 3037 3037
Partials 548 548
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| SearchFields: []string{ | ||
| "sq.tag->>'namespace'", | ||
| "sq.tag->>'key'", | ||
| "sq.tag->>'value'", |
There was a problem hiding this comment.
Just a question for my understanding, how did you determine these 3 fields should be searched?
There was a problem hiding this comment.
maybe @MichaelMraka or @TenSt can double check but this lgtm! Working locally for me
Summary
Allow
/tagsto filter returned tags by matching the search term against tag namespace, key, and value.With this the
/tagsendpoint could be used in the frontend for tag filtering.Summary by Sourcery
Add search support to the system tags listing endpoint and document the new query parameter.
New Features:
Documentation:
Tests: